Add Vizard visualization support for MuJoCo multi-body scenes#1287
Add Vizard visualization support for MuJoCo multi-body scenes#1287ReeceHumphreys wants to merge 6 commits intodevelopfrom
Conversation
ReeceHumphreys
force-pushed
the
feature/mujoco-vizard
branch
from
d2bc5fc to
e553971
Compare
|
@juan-g-bonilla definitely want your feedback on this as the resident MuJoCo guy. Want to make sure not doing anything dumb here haha. |
ReeceHumphreys
force-pushed
the
feature/mujoco-vizard
branch
from
e553971 to
ffa76e5
Compare
src/utilities/vizSupport.py
Outdated
| # Position offset in body frame | ||
| offset = list(geom.pos) | ||
|
|
||
| # Convert quaternion (w, x, y, z) to 3-2-1 Euler angles (z, y, x) |
There was a problem hiding this comment.
Consider using EP2Euler321 from RigidBodyKinematics.py (but double check that function is doing what you expect!).
juan-g-bonilla
left a comment
juan-g-bonilla
left a comment
There was a problem hiding this comment.
I have been summoned! Great PR. I left a lot of comments because reviewing PRs is my happy place, but it's mostly about how I would write some code rather than any "mistakes". Feel free to accept, ignore, or argue about any of my comments (I love an argument).
ReeceHumphreys
force-pushed
the
feature/mujoco-vizard
branch
from
ffa76e5 to
7d32601
Compare
ReeceHumphreys
force-pushed
the
feature/mujoco-vizard
branch
2 times, most recently
from
39092a8 to
aee2fb1
Compare
enableUnityVisualization now accepts MJScene objects in scList.
ReeceHumphreys
force-pushed
the
feature/mujoco-vizard
branch
from
aee2fb1 to
6bc5dec
Compare
juan-g-bonilla
left a comment
juan-g-bonilla
left a comment
There was a problem hiding this comment.
Great work as usual!
| std::vector<double> size = std::vector<double>(3); ///< Size parameters (3 elements). | ||
| std::vector<double> pos = std::vector<double>(3); ///< Position in body frame (3 elements). | ||
| std::vector<double> quat = std::vector<double>(4); ///< Quaternion in body frame (4 elements: w, x, y, z). | ||
| std::vector<double> rgba = std::vector<double>(4); ///< Color (4 elements, 0-1 range). |
There was a problem hiding this comment.
You can do:
std::vector<double> size(3);
if you want shorter syntax
There was a problem hiding this comment.
Also, what was the rational for keeping these as std::vector instead of std::array?
There was a problem hiding this comment.
That syntax suggestion is incorrect in this context. That is only valid for local variable declarations, not in-class default member initializers. The {3} would be parsed as an initializer-list constructor call producing a 1-element vector containing 3.0, not a 3-element zero-initialized vector.
There was a problem hiding this comment.
The reason for the vector is to be consistent with SWIG. SWIG exposes MJGeomInfo to Python via %template() std::vector<double>. Would be incorrect to do an array.
There was a problem hiding this comment.
You're right:
std::vector<double> size(3);
actually makes for some pretty vexing parsing.
Still, if you wanted to use std::array you could do it in SWIG by doing:
%include "swig_std_array.i"
Thanks @juan-g-bonilla , I appreciate your SWIG expertise! |
juan-g-bonilla
left a comment
juan-g-bonilla
left a comment
There was a problem hiding this comment.
Thanks for the fixes!
| freeBodyNames = [name for name in bodyNames if sc.getBodyParentName(name) == "world"] | ||
| if not freeBodyNames: |
There was a problem hiding this comment.
Could rename variable to: topLevelBodyNames (and other variables below that still refer to these as "free".
2 participants
Description
TODO
Verification
TODO
Documentation
TODO
Future work
TODO